Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add check for non-cellstr cell types #463

Merged
merged 1 commit into from
Sep 6, 2022
Merged

Add check for non-cellstr cell types #463

merged 1 commit into from
Sep 6, 2022
+5 −0

Conversation

lawrence-mbf
Copy link
Collaborator

Motivation

One potential misconception about ragged array before may cause a type corruption on write. In the case of #462, addRow supports multi-ragged array appending behavior via use of cell arrays. This is not true for regular instantiation. Regular constructor instantiation is intended for purely raw data initialization.

Users who wish to emulate the behavior in addRow should actually construct the vector index objects themselves if they wish to construct dyamic tables using the default constructor.

In the case of MatNWB, cell arrays passed in this way must error, otherwise silent data corruption can occur.

How to test the behavior?

metadataBlocks = mat2cell(rand(10,13), zeros(1, 5) + 2, ones(1, 13));
metadata = cell(1,13);
for iColumn = 1:13
    metadata{iColumn} = metadataBlocks(:,iColumn);
end
metadata = table(metadata{:});

dt = types.hdmf_common.DynamicTable( ...
    'colnames', {'cluster_id','local_cluster_id','type',...
    'peak_channel_index','peak_channel_id',... % Provide the column order. All column names have to be defined below
    'local_peak_channel_id','rel_horz_pos','rel_vert_pos',...
    'isi_violations','isolation_distance','area','probe_id',...
    'spike_times','spike_times_index'}, ...
    'description', 'Units table', ...
    'id', types.hdmf_common.ElementIdentifiers( ...
    'data', 0:(height(metadata) - 1)), ...
    'cluster_id', types.hdmf_common.VectorData( ...
    'data', metadata{:,1}, ...
    'description', 'Unique cluster id'), ...
    'local_cluster_id', types.hdmf_common.VectorData( ...
    'data', metadata{:,2}, ...
    'description', 'Local cluster id on the probe'), ...
    'type', types.hdmf_common.VectorData( ...
    'data', metadata{:,3}, ...
    'description', 'Cluster type: unit vs mua'), ...
    'peak_channel_index', types.hdmf_common.VectorData( ...
    'data', metadata{:,4}, ...
    'description', 'Peak channel row index in the electrode table'), ...
    'peak_channel_id', types.hdmf_common.VectorData( ...
    'data', metadata{:,5}, ...
    'description', 'Unique ID of the channel with the largest cluster waveform amplitude'), ...
    'local_peak_channel_id', types.hdmf_common.VectorData( ...
    'data', metadata{:,6}, ...
    'description', 'Local probe channel with the largest cluster waveform amplitude'), ...
    'rel_horz_pos', types.hdmf_common.VectorData( ...
    'data', metadata{:,7}, ...
    'description', 'Probe-relative horizontal position in mm'), ...
    'rel_vert_pos', types.hdmf_common.VectorData( ...
    'data', metadata{:,8}, ...
    'description', 'Probe tip-relative vertical position in mm'), ...
    'isi_violations', types.hdmf_common.VectorData( ...
    'data', metadata{:,9}, ...
    'description', 'Interstimulus interval violations (unit quality measure)'), ...
    'isolation_distance', types.hdmf_common.VectorData( ...
    'data', metadata{:,10}, ...
    'description', 'Cluster isolation distance (unit quality measure)'), ...
    'area', types.hdmf_common.VectorData( ...
    'data', metadata{:,11}, ...
    'description', ['Brain area where the unit is located. Internal thalamic' ...
    'nuclei divisions are not precise, because they are derived from unit locations on the probe.']), ...
    'probe_id', types.hdmf_common.VectorData( ...
    'data', metadata{:,12}, ...
    'description', 'Probe id where the unit is located'), ...
    'electrode_group', types.hdmf_common.VectorData( ...
    'data', metadata{:,13}, ...
    'description', 'Recording channel groups'));

fid = H5F.create('test.h5');
refs = dt.export(fid, '/', {});
H5F.close(fid);

Should throw an error. Not write the file.

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #463 (ef43e87) into master (3b26777) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
+ Coverage   87.48%   87.61%   +0.13%     
==========================================
  Files         127      127              
  Lines        5232     5240       +8     
==========================================
+ Hits         4577     4591      +14     
+ Misses        655      649       -6
Impacted Files Coverage Δ
+io/mapData2H5.m 97.50% <100.00%> (+0.27%) ⬆️
+io/getBaseType.m 88.23% <0.00%> (-5.89%) ⬇️
+file/mapType.m 91.42% <0.00%> (-4.03%) ⬇️
NwbFile.m 82.48% <0.00%> (-0.73%) ⬇️
+file/Dataset.m 97.26% <0.00%> (-0.04%) ⬇️
+types/+util/checkDtype.m 83.33% <0.00%> (+1.23%) ⬆️
+types/+util/correctType.m 89.68% <0.00%> (+1.68%) ⬆️
+io/getMatTypeSize.m 88.88% <0.00%> (+22.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lawrence-mbf lawrence-mbf marked this pull request as ready for review September 6, 2022 18:57
@lawrence-mbf lawrence-mbf merged commit cd2baec into master Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@lawrence-mbf